HADOOP-18801. Delete path directly when it can not be parsed in trash.#5744
Merged
Hexiaoqiao merged 8 commits intoapache:trunkfrom Jul 16, 2023
Merged
HADOOP-18801. Delete path directly when it can not be parsed in trash.#5744Hexiaoqiao merged 8 commits intoapache:trunkfrom
Hexiaoqiao merged 8 commits intoapache:trunkfrom
Conversation
hfutatzhanghb
force-pushed
the
HDFS-17046
branch
from
befcfea to
aec9e59
Compare
hfutatzhanghb
force-pushed
the
HDFS-17046
branch
from
aec9e59 to
078150c
Compare
Member
Author
|
@Hexiaoqiao @ayushtkn Hi, Sir, could you please help me to review this PR if you have bandwidth? Thanks a lot. |
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
Member
Author
|
fix checkstyle warning. |
|
🎊 +1 overall
This message was automatically generated. |
Contributor
|
@hfutatzhanghb Thanks to involve me here. I am not sure if you could solve the issue as HDFS-17046 mentioned. I think we should choose one of the following ways, |
Member
Author
@Hexiaoqiao , Sir, thanks a lot for your suggestions. Agree with the second approach and I will fix it soonly. Thanks again. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
hfutatzhanghb
force-pushed
the
HDFS-17046
branch
from
87479ab to
54c9366
Compare
|
🎊 +1 overall
This message was automatically generated. |
Hexiaoqiao
reviewed
Jun 20, 2023
Contributor
Hexiaoqiao
left a comment
Hexiaoqiao
left a comment
There was a problem hiding this comment.
Thanks @hfutatzhanghb for your works. leave some comments inline. JFYI. Please let me know if something I missed.
| LOG.warn("Unexpected item in trash: "+dir+". Ignoring."); | ||
| continue; | ||
| if (cleanNonCheckpointUnderTrashRoot) { | ||
| this.moveToTrash(path, true); |
Contributor
There was a problem hiding this comment.
Why move to trash again since it has been already under Trash home AND has configed to clean? IMO, it is safe to delete them directly.
| */ | ||
| public abstract boolean moveToTrash(Path path) throws IOException; | ||
| */ | ||
| public abstract boolean moveToTrash(Path path, boolean force) throws IOException; |
Contributor
There was a problem hiding this comment.
Sorry I don't get meaning of the new parameter boolean force.
| * @throws Exception | ||
| */ | ||
| @Test | ||
| public void testTrashEmptierWithNonCheckpointDir() throws Exception { |
Contributor
There was a problem hiding this comment.
This new unit test seems not cover the case you try to fix, right? It could pass with/without this improvement.
Contributor
There was a problem hiding this comment.
For this unit test, IMO, we should enable this config and then mkdir one path under Trash home directly, then wait to clean by Emptier. JFYI.
|
🎊 +1 overall
This message was automatically generated. |
Member
Author
|
@Hexiaoqiao Sir, could you please take a look at this PR when you have free time? Thanks a lot. |
|
💔 -1 overall
This message was automatically generated. |
Hexiaoqiao
reviewed
Jul 12, 2023
Contributor
Hexiaoqiao
left a comment
Hexiaoqiao
left a comment
There was a problem hiding this comment.
Leave some comments inline. FYI.
| continue; | ||
| if (cleanNonCheckpointUnderTrashRoot) { | ||
| fs.delete(path, true); | ||
| LOG.warn("Unexpected item in trash: " + dir + ". Force moving to trash."); |
Contributor
There was a problem hiding this comment.
Force moving to trash. -> Force to delete it.
| conf.setClass("fs.file.impl", TestLFS.class, FileSystem.class); | ||
| FileSystem fs = FileSystem.getLocal(conf); | ||
| conf.set("fs.defaultFS", fs.getUri().toString()); | ||
| conf.setBoolean(FS_TRASH_CLEAN_TRASHROOT_ENABLE_KEY, true); |
Contributor
There was a problem hiding this comment.
I do not think we need to update this configuration item here. (L582,L639,L649, etc)
| * @throws Exception | ||
| */ | ||
| @Test | ||
| public void testTrashEmptierWithNonCheckpointDir() throws Exception { |
Contributor
There was a problem hiding this comment.
For this unit test, IMO, we should enable this config and then mkdir one path under Trash home directly, then wait to clean by Emptier. JFYI.
| } | ||
| assertTrue(val == 0); | ||
| } | ||
| Thread.sleep(18000); |
Contributor
There was a problem hiding this comment.
Please try to use GenericTestUtils.waitFor rather than Thread.sleep.
hfutatzhanghb
force-pushed
the
HDFS-17046
branch
from
e6a59b1 to
1bf64b2
Compare
hfutatzhanghb
force-pushed
the
HDFS-17046
branch
from
1bf64b2 to
dcf58a7
Compare
hfutatzhanghb
changed the title
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
Hexiaoqiao
approved these changes
Jul 14, 2023
Contributor
Hexiaoqiao
left a comment
Hexiaoqiao
left a comment
There was a problem hiding this comment.
LGTM. +1. Let's wait if @ayushtkn will give furthermore comments before checkin.
ayushtkn
approved these changes
Jul 14, 2023
hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/TestTrash.java
Outdated
Show resolved
Hide resolved
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/TrashPolicyDefault.java
Outdated
Show resolved
Hide resolved
hadoop-common-project/hadoop-common/src/main/resources/core-default.xml
Outdated
Show resolved
Hide resolved
|
🎊 +1 overall
This message was automatically generated. |
ayushtkn
approved these changes
Jul 15, 2023
Member
ayushtkn
left a comment
ayushtkn
left a comment
There was a problem hiding this comment.
LGTM.
Thanx @Hexiaoqiao for holding, nothing more from my side, can be pushed :-)
Contributor
|
Committed to trunk. Thanks @hfutatzhanghb for your contribution and @ayushtkn for your reviews! |
Member
Author
|
@Hexiaoqiao @ayushtkn Thanks sir for reviewing and merging. |
jiajunmao
pushed a commit
to jiajunmao/hadoop-MLEC
that referenced
this pull request
Feb 6, 2024
apache#5744). Contributed by farmmamba. Signed-off-by: Ayush Saxena <ayushsaxena@apache.org> Signed-off-by: He Xiaoqiao <hexiaoqiao@apache.org>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description of PR
Please see HADOOP-18801.
How was this patch tested?
Add an UT.